Skip to content

fix(wrap): prepend rtk bin dir to PATH for wrapped tools#1752

Open
ketpatil77 wants to merge 4 commits into
headroomlabs-ai:mainfrom
ketpatil77:codex/headroom-rtk-path-launch
Open

fix(wrap): prepend rtk bin dir to PATH for wrapped tools#1752
ketpatil77 wants to merge 4 commits into
headroomlabs-ai:mainfrom
ketpatil77:codex/headroom-rtk-path-launch

Conversation

@ketpatil77

@ketpatil77 ketpatil77 commented Jul 3, 2026

Copy link
Copy Markdown

Description

Make wrapped tools able to resolve Headroom-managed rtk without requiring users to manually add ~/.headroom/bin to PATH.

headroom wrap now prepends the managed rtk bin directory to the child process PATH before launching the tool.

Closes #1752

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • headroom/cli/wrap.py: added case-insensitive PATH-key resolution so Windows-style Path entries are updated in place.
  • headroom/cli/wrap.py: keeps duplicate prevention on the resolved path key instead of creating a second PATH entry.
  • tests/test_cli/test_wrap_helpers.py: added regression coverage for Windows-style env maps and duplicate prevention.

Testing

  • Unit tests pass (pytest)
  • Linting passes (ruff check .)
  • Type checking passes (mypy headroom)
  • New tests added for new functionality
  • Manual testing performed

Test Output

$ C:\Users\Jayesh\AppData\Local\Programs\Python\Python314\python.exe -m pytest tests\test_cli\test_wrap_helpers.py -k "prepend_rtk_bin_to_path"
3 passed, 37 deselected in 1.14s

$ ruff check headroom\cli\wrap.py tests\test_cli\test_wrap_helpers.py
All checks passed!

Real Behavior Proof

  • Environment: Windows 11, PowerShell, Python 3.14, Headroom wrap helper path-prepend flow.
  • Exact command / steps: Ran _prepend_rtk_bin_to_path() regression tests against both PATH and Windows-style Path env dictionaries, plus duplicate-path coverage.
  • Observed result: Managed rtk bin dir prepends exactly once and updates existing Path key in place without creating a second differently-cased entry.
  • Not tested: Full interactive wrapped-shell launch against a live Windows child process in this verification pass.

Review Readiness

  • I have performed a self-review
  • This PR is ready for human review

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the CHANGELOG.md if applicable

Additional Notes

  • Follow-up push already addressed reviewer concern about case-insensitive Windows PATH handling; this body update clears governance/template blockers.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

PR governance

This PR follows the template and is marked ready for human review.

@github-actions github-actions Bot added the status: needs author action Pull request body or readiness checklist still needs author updates label Jul 3, 2026

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PATH injection is the right place to solve this, but this helper needs a Windows-safe environment-key handling pass before merge.

env comes from os.environ.copy() in the wrappers, and on Windows that commonly carries the executable search path under Path, not PATH. _prepend_rtk_bin_to_path() currently reads env.get("PATH", "") and writes env["PATH"], which can leave the existing Path entry untouched and add a second differently-cased key. With subprocess.run(..., env=env) on Windows, duplicate case variants are ambiguous and can result in the child still seeing the old Path, so the managed rtk bin directory may not actually be on the child search path.

Please resolve the existing path key case-insensitively, update that key in place, and add tests for a Windows-style env like { "Path": "C:\\Windows\\System32" } so we know the helper prepends to Path rather than creating a separate PATH. It would also be worth asserting the duplicate check works against the resolved key.

@ketpatil77

Copy link
Copy Markdown
Author

Pushed follow-up fix for review: resolve existing PATH/Path key case-insensitively, update that key in place, and add Windows-style env regression coverage. Re-ran targeted tests and ruff locally before push.

@github-actions github-actions Bot added status: ready for review Pull request body is complete and the author marked it ready for human review and removed status: needs author action Pull request body or readiness checklist still needs author updates labels Jul 4, 2026
@ketpatil77

Copy link
Copy Markdown
Author

Follow-up is already pushed for the requested Windows PATH/Path fix: the helper now resolves the existing path key case-insensitively, updates that key in place, and includes Windows-style env regression coverage plus duplicate-path coverage. Targeted pytest and ruff outputs are in the PR body. Re-review requested when convenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: ready for review Pull request body is complete and the author marked it ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants